-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SorobanDataBuilder to prepare sorobanData easily. #509
Add SorobanDataBuilder to prepare sorobanData easily. #509
Conversation
* @param metadataBytes number of extended metadata bytes (uint32) | ||
* @return this builder instance | ||
*/ | ||
public SorobanDataBuilder setResources( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because all the parameters have the same type I think it could lead to mistakes where you pass in the parameters in the wrong order. Is it possible to have a Resources builder object that could be passed into setResources()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, fixed in 9f5f231
* sorobanData | ||
* @return this builder instance | ||
*/ | ||
public SorobanDataBuilder setFootprint( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because all the parameters have the same type I think it could lead to mistakes where you pass in the parameters in the wrong order. Is it possible to have a footprint builder object that could be passed into setFootprint()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If necessary, we can remove this method and only keep setReadOnly
and setReadOnly
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's an even better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4da8f81
Closes #508